-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2627 fix "some of" golangci-lint problems #2641
Conversation
0a028c3
to
1a1b7c8
Compare
contrib/cmd/recvtty/recvtty.go
Outdated
os.Exit(1) | ||
} | ||
|
||
func handleSingle(path string, noStdin bool) error { | ||
func handleSingle(path string, noStdin bool) (err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest calling it retErr
or so, to distinguish between errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err
is what I usually use.
contrib/cmd/recvtty/recvtty.go
Outdated
// Open a socket. | ||
ln, err := net.Listen("unix", path) | ||
if err != nil { | ||
return err | ||
} | ||
defer ln.Close() | ||
defer func() { | ||
err = ln.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what would happen if net.Listen
fails? Please be careful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong here (except that you rewriting the error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to metion the defer part will run after func returns. so if any error happens in closing, the retErr
will be refilled. the question is: is it important to you to handle this kind of errors in the caller function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also another problem: if the func returns because of another error, and at the same time, the close also fails, the former error will be lost! while the closing error is bound (in my opinion) I believe the original error is more significant ... tell me what to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to ignore closing error...
contrib/cmd/recvtty/recvtty.go
Outdated
|
||
// We only accept a single connection, since we can only really have | ||
// one reader for os.Stdin. Plus this is all a PoC. | ||
conn, err := ln.Accept() | ||
if err != nil { | ||
return err | ||
} | ||
defer conn.Close() | ||
defer func() { | ||
err = conn.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several changes of this form, and while Close
can fail very few Go programs handle this case successfully all the time and you would need to write the same boilerplate for each case:
defer func() {
if Err != nil {
Err = foo.Close()
}
}()
Aside from adding a bunch of boilerplate to lots of functions, the second question is when will close(2)
fail and does it make sense to deal with this? Most close failures are actually when the filesystem had an error writing to disk, but this mechanism in Linux is unpredictable and ultimately there's not much to do if that's why close(2)
failed (very often the failure happens during a later fsync(2)
). Most other close(2)
failures don't apply to most Go programs. Not to mention that the worst thing that happens is that we keep a file descriptor open in a program which takes about 100ms to run -- and we can't do anything about it anyway because the semantics of close(2)
are that you cannot safely attempt to re-close a file after calling close(2)
.
We can do it, but I don't see the benefit and if the only reason for this change is to make a CI lint happy, we should turn off the CI lint IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the alternative that is not too much fuss and makes static checkers happy is same as in (void)write();
in C, i.e.
defer func() {
_ := foo.Close()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your expressive description. As a boilerplate, I may suggest this one:
defer func() {
if err = foo.Close(); err != nil && retErr != nil {
retErr = err
}
}()
because we should try to close() anyways. but its much more than its needed (I think), and as @kolyshkin said, I'll try to simply ignore the closing error across my MR
@@ -61,28 +61,35 @@ terminals: | |||
) | |||
|
|||
func bail(err error) { | |||
fmt.Fprintf(os.Stderr, "[recvtty] fatal error: %v\n", err) | |||
_, _ = fmt.Fprintf(os.Stderr, "[recvtty] fatal error: %v\n", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not inline with what the commit message says. Please split your commits carefully.
Thank you for PR. I left a few suggestions. In general, I would like it to be split into smaller focused patches (I see you already did that), and make sure your code is correct. |
1a1b7c8
to
81a080f
Compare
Sorry for being late, I didn't receive any email or notif. I should have configured github before. thanks |
c4d3374
to
56b1aa8
Compare
Signed-off-by: Amir Mohamad Ehsandar <[email protected]>
Signed-off-by: Amir Mohamad Ehsandar <[email protected]>
Signed-off-by: Amir Mohamad Ehsandar <[email protected]>
Signed-off-by: Amir Mohamad Ehsandar <[email protected]>
I tried to apply your suggestions. and I tried to split my commits a little more. sorry for making mistakes. It's the first time I contribute to an open source repo. I really didn't expect this precise review. thank you. |
} | ||
if err := console.ClearONLCR(c.Fd()); err != nil { | ||
return err | ||
} | ||
|
||
// Copy from our stdio to the master fd. | ||
quitChan := make(chan struct{}) | ||
errChan := make(chan error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK it seems that in this case a single channel is enough, there is no need to have two.
return err | ||
ln, retErr := net.Listen("unix", path) | ||
if retErr != nil { | ||
return retErr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did't have to change any of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOW this can stay err
-- the only place to use retErr
is the defer.
return err | ||
conn, retErr := ln.Accept() | ||
if retErr != nil { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
return err | ||
socket, retErr := unixconn.File() | ||
if retErr != nil { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
return err | ||
c, retErr := console.ConsoleFromFile(master) | ||
if retErr != nil { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
_, err := io.Copy(os.Stdout, c) | ||
if err != nil { | ||
errChan <- err | ||
} | ||
quitChan <- struct{}{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOW this could be simple
_, err := io.Copy(os.Stdout, c)
chan <- err
and the code that's receiving this will check for nil
@ehsundar please let us know if you're going to keep working on that or not |
@kolyshkin Would you be interested in carrying this PR? 🙏 |
Looks like I have no choice :) so yes, I will :) |
Thanks! |
Not using any patches from this one since it's easier to fix from scratch, thus closing. The issue #2627 is still open though. |
superseded by the above PRs |
this request is related to this issue